-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: support @web-std/file in normalize input #3750
fix: support @web-std/file in normalize input #3750
Conversation
1dae15c to
c5e02da
Compare
| */ | ||
| function isBlob (obj) { | ||
| return typeof Blob !== 'undefined' && obj instanceof Blob | ||
| return obj.constructor && (obj.constructor.name === 'Blob' || obj.constructor.name === 'File') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this prevent an instance of any old class named "File" or "Blob" being detected as a File or Blob?
Maybe ensure that methods are present on obj that we will call later on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this sounds a good idea
|
|
||
| /** | ||
| * @param {any} obj | ||
| * @returns {obj is Blob} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the const { Blob } = ... line was removed, this is now the global Blob so TS will be unhappy if someone explicitly passes @web-std/file objects in your code.
Might just need to remove the @returns annotation so tsc will derive a boolean return type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return the @returns causes typescript to also not be happy, I added globalThis to the type
|
|
||
| if (isBrowser || isWebWorker || isElectronRenderer) { | ||
| expect(content).to.be.an.instanceOf(Blob) | ||
| try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File extends Blob so under what conditions would we need to fall back here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instanceOf is tricky.
File https://github.com/web-std/file/blob/default/src/lib.js#L6 extends from Blob as you mention, and Blob is https://github.com/web-std/io/blob/main/blob/src/lib.js (so the same as here).
But, somehow this will not work with instanceOf Blob, and works with @web-std/blob. So, if you create a Blob and a @web-std/blob, their instanceOf are not compatible...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds like @web-std/blob needs to return the native implementation if it's available?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also of note here is that node 16 has a Blob implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds like @web-std/blob needs to return the native implementation if it's available?
It does: https://github.com/web-std/io/blob/main/blob/src/lib.js
but instanceOf returns false 🤷🏼♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it now to Blob to be:
const { Blob } = require('@web-std/blob')instead of
const { Blob } = globalThisIt works this way, but then other tests fail (which probably use the native Blob 🤷🏼♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverting this, as this is even more problematic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, when creating a https://github.com/web-std/io/tree/main/file in the browser, it will not be an instance of Blob, but it will be of https://github.com/web-std/io/tree/main/blob. This is really weird, given that ws Blob is really globalThis.Blob.
This codepen shows that a browser File should be instance of both Blob and ws Blob.
My only assumption for this to not be the case in the tests here, is that somehow playwright has a different globalThis scope in the test that makes them being different... @hugomrdias is this reasonable? do you have any clues?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
web-std/io#10 this fixed the issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now @web-std/file uses the Node.js version with Eletron renderer 😡
feb1a2f to
d09c419
Compare
d09c419 to
5f798ef
Compare
Related to storacha/ipfs-car#62